Skip to content

fix: percent-encode resource IDs in ARI URL path segments#21

Open
deadcode-walker wants to merge 15 commits intomainfrom
fix/codebase-review-hardening
Open

fix: percent-encode resource IDs in ARI URL path segments#21
deadcode-walker wants to merge 15 commits intomainfrom
fix/codebase-review-hardening

Conversation

@deadcode-walker
Copy link
Owner

Summary

  • Wrap self.id with url_encode() in all format! calls that interpolate resource IDs into URL path segments across channel.rs, bridge.rs, and playback.rs
  • Also encode playback_id and operation parameters that appear in path segments
  • Add 7 unit tests verifying url_encode handles path-unsafe characters (/, ?, #, %, space, &) and preserves safe characters

Test plan

  • cargo test -p asterisk-rs-tests --test unit — 894 tests pass (including 7 new url_encode tests)
  • cargo test -p asterisk-rs-tests --test mock_integration — 210 tests pass
  • cargo clippy --workspace --all-targets --all-features -- -D warnings — clean
  • cargo fmt --all -- --check — clean

Added project-level CLAUDE.md with agent instructions for test routing
and CI requirements. Gitignored .claude/ for local harness config.
Removed obsolete .omp/ rules and skills (replaced by AGENTS.md and
CLAUDE.md).
…rver_ok

Register the Notify future before checking the atomic counter to prevent
a TOCTOU race that could hang tests under load. Remove redundant
Arc<Notify> inside Arc<ServerState>. Move assert_server_ok to shared
helpers and replace all remaining `let _ = handle.await` in mock tests.
CRITICAL:
- fix unbounded recursion in AmiCodec::decode on empty frames
- replace derived Debug on AriConfig with manual impl that redacts
  password and ws_url

HIGH:
- adopt Credentials (Zeroizing<String>) from core for ARI config,
  replacing plain String username/password fields
- add require_challenge option to AmiClientBuilder (default true)
  to prevent silent plaintext auth fallback
- restrict AMI connection module to pub(crate) visibility
- eliminate unnecessary AmiResponse clone in dispatch_message
- add AgiError::InvalidConfig variant (was shoehorned into Io)

MEDIUM:
- apply url_encode to all user-supplied ARI resource path segments
  (mailbox, device_state, sound, recording, asterisk modules)
- extract duplicated WsRestRequest into shared ws_proto module
- send WebSocket close frame on ARI listener shutdown
- fix weak jitter entropy in reconnect backoff (hash ThreadId)
- downgrade raw ARI payload logging from WARN to TRACE
- restrict ws_url() accessor to pub(crate) to prevent credential leak
- make AriConfig fields pub(crate) with read-only accessors
- document tokio::sync::Mutex hold behavior in Call::wait_for_answer
Fixes CRL distribution point matching vulnerability where correct CRLs
were not consulted for revocation checking on certs with multiple
distributionPoints.
- semver.yml: add continue-on-error (release-plz owns version bumps)
- ci.yml: pin typos action to v1 instead of master
- docs.yml: use taiki-e/install-action@mdbook instead of cargo install
- coverage.yml: enforce --fail-under-lines 60 minimum
- dependabot.yml: add rust ecosystem for toolchain updates (monthly)
- deny.toml: wildcards "allow" -> "deny" to block wildcard deps
- coverage.yml: remove --fail-under-lines (threshold meaningless with
  external test crate) and include test crate in coverage measurement
- dependabot.yml: remove rust ecosystem (no rust-toolchain.toml exists)
- security.yml: add pull_request trigger so Cargo Deny reports on PRs
  (was only push+schedule, causing required check to never report)
- deny.toml: skip-tree asterisk-rs-tests (internal crate uses workspace
  wildcards, never published)
Channel, bridge, and playback IDs containing path-unsafe characters
(/, ?, #, etc.) would corrupt request URLs. Apply url_encode() to
self.id in every format! that interpolates it into a path segment.
Also encode playback_id and operation parameters in bridge and
playback handles.
@github-actions github-actions bot added documentation Improvements or additions to documentation dependencies Pull requests that update a dependency file ci ami agi ari core labels Mar 23, 2026
- Make LoginAction.secret private with Zeroizing<String> to prevent
  secret leakage in memory and debug output (SEC-011)
- Add LoginAction::new() constructor and Debug impl with [REDACTED]
- Fix is_follows_response to handle missing space after colon,
  e.g. "Response:Follows" (PROTO-004)
- Truncate banner content to 64 bytes in error messages (ENG-004)
- Add atomic ordering comment on ACTION_ID_COUNTER (ENG-005)
- Update all LoginAction call sites to use constructor
…edia events

- Replace DefaultHasher+SystemTime jitter with RandomState (OS-seeded entropy)
  to avoid poor randomness on boot or shared-thread scenarios (ENG-003)
- Add AtomicU64 dropped_count to CallTracker for observability on channel
  backpressure (CONC-010, CONC-011)
- Use .send().await for critical MediaEvent variants (MediaStart,
  MediaBufferingCompleted) to prevent dropping session-critical control
  events while keeping try_send for non-critical events (CONC-012)
- Update jitter range tests from [0.5, 1.0] to [0.5, 1.5) to match new
  implementation
…l state

- Escape backslashes in quoted AGI command arguments (SEC-008)
- Fix nested parentheses truncation using rfind (PROTO-009)
- Limit read_line to 8193 bytes via take() to prevent OOM (PROTO-005)
- Default server bind to 127.0.0.1 instead of 0.0.0.0 (SEC-005)
- Track spawned connection tasks with Vec<JoinHandle> (ENG-009)
- Set ChannelState::Poisoned on EOF/hangup instead of Ready (CONC-007)
- Check hung_up before state in send_command for correct error priority
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agi ami ari ci core dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant